Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial Config Management Design #87

Closed
wants to merge 5 commits into from

Conversation

danehans
Copy link
Contributor

@danehans danehans commented Jun 2, 2022

Adds a doc that details the initial config management design.

xref #43

@danehans danehans marked this pull request as draft June 2, 2022 20:20
Comment on lines 169 to 171
// TODO: A Manager requires a rest config to construct a clutser. This means
// using Manager to manage services requires access to a k8s cluster.
// xref: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.12.1/pkg/cluster/cluster.go#L145-L205
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@youngnick PTAL at this. I see this as an issue for using Runnable since we need to support non-k8s environments in the future. cc: @arkodg

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recording some notes from the discussion in the community meeting:

  • @danehans is going to look into another framework, and make sure it has Context support.
  • the rest of the PR is reviewable already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the call:

So it sounds like we're thinking that we'll need to have two goroutine managers: the controller-runtime one and a non-kube one, possibly nested. IMO, https://pkg.go.dev/github.com/datawire/dlib/dgroup is attractive for this because it takes a func(ctx context.Context) error function pointer as the argument, which happens to be the same signature as controller-runtime's manager.Runnable's Start method; instead of saying manager.Add(myRunnable), you'd say group.Go("name", myRunnable.Start).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer using https://github.com/tetratelabs/run which also provides the ability to use context for life cycle management of go routines. It provides a superset of functionality compared to dgroup and specifically also provides a notion of readiness through different stages of a routine lifecycle, which dgroup omits. This due to it being more than just go routine management but even more so a reusable component manager. It allows components to have their own configuration management based on https://github.com/spf13/pflag and provides a dedicated deterministically ordered pre-run wiring and set-up phase next to routine start/stop.

Based on it we (at Tetrate) have a bunch of components we can easily reuse and have configured in a consistent manner (e.g. gRPC server. HTTP server, Signal handler, etc.) Due to strong guarantees on lifecycle management it makes things as registering add-on gRPC services to a gRPC server easy and straight forward. Given we want EG to be highly extensible and easy to wrap/embed by other projects or products I think it would be the right choice.

Copy link
Contributor Author

@danehans danehans Jun 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@basvanbeek thank you for providing additional context for https://github.com/tetratelabs/run. From the tetratelabs/run docs:

... It uses the https://github.com/oklog/run/ package as its basis and enhances it with configuration registration and validation as well as pre-run phase logic.

Have you considered adding config registration, validation, and pre-run logic to oklog/run?

Since we're at an impasse on whether to use https://github.com/tetratelabs/run or https://pkg.go.dev/github.com/datawire/dlib/dgroup, should we proceed with the upstream (oklog/run) that both projects are based on?

@arkodg @LukeShu @alexgervais @youngnick @skriss please provide input so I can proceed with iterating on the design.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer picking one of our two more complete options than going with the upstream oklog/run and having to re-implement parts of functionalities we'll require anyway. I'm familiar with datawire/dlib/dgroup so it would be my go-to choice, but that's only because I haven't worked with tetratelabs/run.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, dgroup is not built on oklog/run. It is built on a forked/vendored version of golang.org/x/sync/errgroup.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danehans I have very briefly considered to upstream the logic of tetratelabs/run when I started building it.

To be honest, oklog/run is a finished library. It serves the scope that @peterbourgon envisioned (goroutine lifecycle management prior to context.Context cancellation being available as standard lib) and is stable (not receiving any updates, doesn't have issues, etc.). The library is very small by itself so it carries more an idea than code.

tetratelabs/run's scope is much bigger. It has native support for context cancellation as well as services that have start/stop semantics. It provides more a framework for reusable components that allow for easy composition and self contained configuration management, bootstrap logic and allows ordered initialization and pre-run phases.

I agree with @alexgervais that choosing native oklog/run is a bad idea. However, as I expressed above, I would prefer tetratelabs/run over dgroup specifically for its inclusion of deterministically ordered configuration and pre goroutine run stages (things specifically mentioned to not be dealt with by dgroup.

The reason I request for these additional features is that it allows the component authors to have consistent handling and management of their components and isolated from others, while keeping a binary's wiring small and clean. This allows projects and Vendor products to reuse as much as possible, having an easy way to embed/wrap EG in their code without having to deal with untangling EG bootstrap or having to duplicate lots of code that will need constant sync with upstream.


## Proposal

Introduce a `serve` argument to the `envoy-gateway` command. The `serve` argument encapsulates the management of Envoy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for a subcommand that spawns and manages all components.

@danehans danehans marked this pull request as ready for review June 6, 2022 18:40
docs/design/CONFIG_MANAGEMENT.md Outdated Show resolved Hide resolved
docs/design/CONFIG_MANAGEMENT.md Outdated Show resolved Hide resolved
@danehans
Copy link
Contributor Author

Closing since #111 and #95 cover config management.

@danehans danehans closed this Jun 27, 2022
@danehans danehans deleted the issue_43 branch July 12, 2022 19:21
zhaohuabing added a commit to zhaohuabing/gateway that referenced this pull request Nov 5, 2024
Signed-off-by: Huabing Zhao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants